Harden boundary delivery and data convergence - #10646
Conversation
24b1a5e to
a90a80c
Compare
Git-on-my-level
left a comment
There was a problem hiding this comment.
Thanks for the very thorough write-up and the proof-fail cases. I reviewed this as a high-risk boundary/data-convergence change set.
Positive signal from this pass: the scope is cohesive around delivery idempotency, ambiguous boundary outcomes, and legacy projection convergence, and I did not find an obvious security/supply-chain concern in the changed surfaces. The approach of stable receiver-visible delivery IDs, bounded Redis fencing/done markers, not claiming synthetic webhook success, over-fetching stale vector hits, and preserving ambiguous task/provider outcomes looks directionally right. I also verified the current GitHub checks are green from the PR checks view, and git diff --check origin/main...HEAD is clean locally.
I’m intentionally not approving because this touches websocket delivery semantics, developer webhooks, user memory/vector convergence, task-provider error classification, and public generated API contracts across a large diff. Before merge, I’d like a human maintainer to review the delivery/ACK semantics and data-convergence tradeoffs in particular: Redis fail-open duplicate behavior, which local completions are acknowledged versus downstream delivery guarantees, and the public API change for retryable/ambiguous task creation results.
No specific code changes requested from this automated pass.
by AI on behalf of David — if you need David’s attention urgently, please @Git-on-my-level and escalate with need human response.
tianmind-studio
left a comment
There was a problem hiding this comment.
I found one actionable stale-socket race in the reconnect path that should be fixed before merge.
[P1] Fence receive failures by the socket that actually failed — backend/utils/listen_pusher_session.py:584
pusher_ws is captured before recv(), but the ConnectionClosed branch calls session-wide _mark_disconnected() without checking that the failing socket is still current. A concurrent send failure can mark the old socket down and install a replacement; if the old recv() is scheduled to raise afterward, this branch flips pusher_connected to False for the healthy replacement and starts another reconnect that closes it.
I reproduced this with a blocked old recv(), session.connect() installing a replacement, then releasing the old ConnectionClosed: the replacement remained in pusher_ws, but pusher_connected became false. Switching this call to _mark_failed_socket_disconnected(pusher_ws, auto_reconnect=True), as the send paths already do, made that regression plus the existing test_listen_pusher_session.py suite pass (26 tests total).
I focused this pass on negotiated transcript/speaker delivery, reconnect replay and close ordering, webhook retry identity/reset behavior, provider outcome classification, and generated client parity. The other inspected boundaries were internally consistent, and the Windows TypeScript client matches the expanded OpenAPI task response.
|
@tianmind-studio I’m coordinating review of this PR directly with the Omi maintainers. Please leave further reviews and change requests on my PRs to maintainers unless I’ve asked you for input, and focus your review activity on your own PRs. I’ll work with the maintainers on any changes they consider necessary here. |
Git-on-my-level
left a comment
There was a problem hiding this comment.
Request changes: socket-identity fencing is incomplete
This PR correctly introduces _mark_failed_socket_disconnected(failed_ws, ...) for most outbound send failures, but two receive/liveness paths still use session-wide _mark_disconnected() after capturing a socket reference. That permits a stale failure from old socket A to mark a healthy replacement B disconnected after B has been installed. The result is an unnecessary reconnect cycle and can close/replace B.
Required implementation
- In
ListenPusherSession.pusher_receive(), replace theConnectionClosedhandler's_mark_disconnected()with:
self._mark_failed_socket_disconnected(
pusher_ws,
auto_reconnect=True,
)-
Apply the same socket-identity fence to
pusher_heartbeat(). It currently calls_mark_disconnected()when an old heartbeat send raisesConnectionClosed, which leaves the same stale-socket race. -
Add a deterministic hermetic regression test in
backend/tests/unit/utils/test_listen_pusher_session.py. Use the existing injectableconnect_to_pusherdependency and controllable fake websockets; do not use timing sleeps as the proof. For each of the receive and heartbeat variants:- begin a blocked operation on old socket A;
- trigger reconnect and install healthy socket B;
- release A so it raises
ConnectionClosed; - assert
session.pusher_ws is B,session.pusher_connected is True, B was not closed, and no redundant reconnect task was created.
The invariant should be explicit: a failure associated with socket A may mutate session connection state only when A is still session.pusher_ws.
This is a focused, hermetic proof of the reported race; the current green gauntlet does not cover this interleaving. Please rerun the focused session suite and the Listen/Pusher Stack Gauntlet on the new PR head after the fix.
## Problem `GET /v1/dev/user/memories` authorized and pinned canonical accounts correctly, but its `MemoryService.read()` call independently reran the stricter rollout/control read decision. When those decisions diverged, the canonical route silently read the legacy collection and returned HTTP 200 with a stale pre-cutover prefix. The MCP list routes had the same double-routing boundary. ## Fix - Add a narrow `MemoryService.read_pinned()` seam that reads the backend already selected by an authorized external list route. - Use that seam for Developer API, MCP REST, and MCP SSE canonical list reads. - Preserve the existing default-read grant ordering, canonical visibility filtering, public response projection, and legacy behavior for legacy-pinned accounts. - Add a behavioral Developer API regression with old- and new-schema canonical rows, including predicate, subject attribution, evidence, veracity, and uncertainty fields. It proves the current row remains visible while raw provenance fields stay outside the Developer response. PR #10646 changes legacy search and mutation/vector boundaries in `memory_service.py`; this PR does not overlap those hunks or duplicate its behavior. ## Product invariants affected - INV-MEM-1 Failure-Class: none This is an isolated double-routing defect rather than an instance of a registered semantic failure class. The prevention artifact is the Developer route regression at `backend/tests/unit/test_dev_api_canonical_grant_ordering.py::test_get_memories_uses_authorized_canonical_pin_for_new_schema_rows`, backed by the explicit pinned-read service seam. ## Verification - Prove-fail: with the Developer route restored to `MemoryService.read()`, the new regression returned only `canonical-old-schema` and failed because `canonical-new-schema` was absent. - `backend/.venv/bin/python -m pytest -q backend/tests/unit/test_dev_api_canonical_grant_ordering.py backend/tests/unit/test_dev_api_memories_pagination.py backend/tests/unit/test_mcp_data_endpoints.py backend/tests/unit/test_ws_l_surface_routing.py backend/tests/unit/test_memory_service_parity.py` — 82 passed. - Diff-selected `backend/test.sh` lane — all 38 selected unit-test files passed. - Pyright on the four changed production modules with the pinned Python 3.11 environment — 0 errors, 0 warnings. - Black 24.4.2 check on all six changed Python files — clean. - `make preflight` — passed after the final commit. Closes SCA-185 <!-- This is an auto-generated description by cubic. --> <a href="https://cubic.dev/pr/BasedHardware/omi/pull/10696?utm_source=github" target="_blank" rel="noopener noreferrer" data-no-image-dialog="true"><picture><source media="(prefers-color-scheme: dark)" srcset="https://www.cubic.dev/buttons/review-in-cubic-dark.svg"><source media="(prefers-color-scheme: light)" srcset="https://www.cubic.dev/buttons/review-in-cubic-light.svg"><img alt="Review in cubic" src="https://www.cubic.dev/buttons/review-in-cubic-dark.svg"></picture></a> <!-- End of auto-generated description by cubic. -->
Git-on-my-level
left a comment
There was a problem hiding this comment.
Thanks for the update. I re-checked the socket-identity blocker from my previous review against head 87917e774ba2edbbf8dc236c021ec65e2b49d80a.
Positive signal: the requested stale-socket fencing is now present in both paths I called out: pusher_receive() calls _mark_failed_socket_disconnected(pusher_ws, auto_reconnect=True) on ConnectionClosed, and pusher_heartbeat() does the same for heartbeat send ConnectionClosed. I also found the focused regression coverage in test_stale_socket_failure_does_not_disconnect_replacement, parameterized over receive and heartbeat, asserting that a stale socket A failure does not disconnect replacement socket B or create a redundant reconnect task.
I’m not requesting further code changes from this pass, and I’m dismissing my stale CHANGES_REQUESTED review because its specific blocker appears resolved on the current head.
Still intentionally not approving: this remains a large, security-sensitive boundary/data-convergence change touching websocket delivery ACK semantics, Redis delivery fencing, developer webhooks, memory/vector convergence, task-provider ambiguity classification, and generated public API contracts. The PR already has security-review and needs-maintainer-review, which still look appropriate; a human maintainer should sign off on the delivery guarantees, fail-open duplicate behavior, and public task response contract before merge.
Validation note: I statically inspected the current head and compiled the changed pusher session source/test files. A focused pytest attempt in a fresh worktree was blocked by this local environment missing backend test dependencies (ModuleNotFoundError: No module named 'google'), so I’m relying on static verification plus the PR’s reported test evidence rather than claiming a local pytest pass.
by AI on behalf of David — if you need David’s attention urgently, please @Git-on-my-level and escalate with need human response.
Current head 87917e7 fences the previously blocking receive and heartbeat stale-socket ConnectionClosed paths, and includes focused regression coverage for both paths. Dismissing only this automation-authored stale review; human/other-actor reviews are preserved.
|
Addressed the maintainer-requested stale-socket race in both receive and heartbeat paths.
David re-checked head |
1cfed1b to
285a4b9
Compare
|
Rebased onto current The deterministic old-socket/replacement-socket regression fails against the byte-identical current-main product file with |
285a4b9 to
4ea54d0
Compare
|
Follow-up CI corrections are in
The PR remains ready and mergeable; a fresh CI run is in progress. |
|
Thanks for the continued work on this. I reviewed the current head ( Positive signal from this pass: I did not find a new blocking issue in the sampled high-impact paths, and the current CI/check set is green. Specific observations:
I am leaving this for human maintainer review because it changes security/privacy-sensitive delivery, webhook, and user-memory persistence boundaries. The implementation looks coherent from this pass, but the merge decision should include maintainer sign-off on the new boundary semantics and rollout risk. — Automated maintainer review (glm-5.2) by AI on behalf of David — if you need David’s attention urgently, please @Git-on-my-level and escalate with |
Resolves three weeks of drift. Kept the webhook delivery-health model, task-integrations hardening, async http and storage infrastructure, and speaker identification delivery. Dropped the memory-stack and pusher-side pieces that upstream superseded: the universal memory and task authority convergence rewrote the memory paths this branch hardened, and PR 10678 landed its own bounded delivery-failure handling for the pusher. Details in the PR comment. Generated clients and the app-client spec regenerated from the merged backend.
|
Rebased this onto current main by merging main in and resolving the conflicts. The PR is MERGEABLE again, and its scope is now narrower than when it was opened, because upstream landed overlapping work while it sat conflicting: Kept (still applies to current main):
Dropped as superseded by upstream work that landed after this branch was cut:
Where a dropped piece still has residual value (for example client-confirmed delivery acks on top of #10678's model), it should be a fresh PR against the current architecture rather than a rebase of this one. Resolution notes: the merged webhook test suite was adapted to the delivery-health model (reset on enable writes a cleared health hash without claiming an HTTP success, so the e2e disable test asserts the health snapshot is unchanged and that a user disable is not the auto-disable flag). Generated clients and the app-client OpenAPI spec were regenerated from the merged backend with the repo scripts; the regenerated output is byte-identical to the merged state, so every remaining generated delta is backed by the surviving API surface. |
…erification The legacy_memory_projection_convergence workflow entry belonged to the memory-stack half this PR dropped as superseded by the universal memory authority: its sources pulled database/memories.py into the tuple-result scan (flagging functions that live identically on main) and its test list referenced the deleted mcp search suite. The surviving vector_db delta was the same dropped surface's write-verification and returned a reported count where main's contract returns the payload length, failing main's batch-upsert tests; the file reverts to main wholesale.
Problem
Several boundary paths treated an attempted send or write as a complete outcome even when the remote effect was unknown or only partially persisted.
At
origin/main, the pusher client returned after a failed finalization send without invalidating the dead socket:Legacy memory search requested only the desired result count before filtering missing, locked, rejected, invalid, or malformed records:
Todoist task creation converted an omitted provider identity into the string
"None"and reported success:The same boundary problem appeared in transcript and speaker delivery, webhook retries, vector projection counts, and legacy X-source acknowledgement after partial projection.
Reachability / failure scenario
backend/utils/listen_pusher_session.py::ListenPusherSession.request_conversation_processing,_transcript_flush,_audio_bytes_flush, andsend_speaker_sample_requestawait socket sends during ordinary disconnect and cancellation paths. A failed socket could remain marked connected, while accepted work was discarded before_pusher_reconnect_loopcould replay it. The fixed speaker path now delegates to_send_speaker_sample_request.backend/routers/pusher.py::_websocket_util_triggerruns transcript and speaker effects inprocess_transcript_queueandprocess_speaker_sample_queue. If the connection closes after one effect completes, the client cannot distinguish completed work from work that must be replayed.backend/utils/speaker_identification.py::extract_speaker_samplesuploads throughbackend/utils/other/storage.py::upload_person_speech_sample_from_bytesbeforeadd_person_speech_samplecommits the reference. A retryable database outcome previously created a new random object for the same logical delivery.backend/utils/memory/memory_service.py::_legacy_search_memoriesrequested onlylimitvector candidates, then hydrated and filtered them. Stale, locked, rejected, invalid, invalidated, or malformed leading hits could therefore hide eligible lower-ranked results.backend/routers/memories.py::_purge_legacy_memoriesenumerated IDs throughbackend/database/memories.py::get_memoriesbefore callingdelete_all_memories. Records excluded by that reader were deleted from Firestore without their vector IDs being available for cleanup.backend/routers/task_integrations.py::create_task_via_integrationdelegates tobackend/utils/task_integrations_ops.py::create_task_internal. A provider 2xx response without an ID became"None", while a connection failure after send was not distinguished from a safe pre-send failure.backend/utils/x_connector.py::_extract_and_indexpersisted legacy memory documents and then calledupsert_memory_vectors_batch. A partial legacy vector write could still let the source be counted as processed and acknowledged.backend/utils/webhooks.py::_post_dev_webhookretried permanent and transient responses alike, andbackend/utils/app_integrations.py::_async_trigger_realtime_integrationslacked stable retry identity. In addition,backend/routers/users.py::enable_user_webhook_endpointrecorded synthetic success without clearing the process-local circuit breaker.Fix
408,425,429, and5xx, while reusing one receiver-visible idempotency key. Explicit re-enable or URL replacement resets persisted and process-local delivery health without recording synthetic success.origin/mainsibling_legacy_search_memories_mcpalready over-fetchedmin(limit * 3, 60)before filtering; this change makes both API and MCP paths progressively backfill instead of relying on one fixed over-fetch.CreateTaskResponse, preserveerror_code,retryable, andambiguousthrough the public response and generated clients. Existing authentication and token-refresh preflight HTTP errors remain unchanged. Ambiguous task creation is reported, not retried automatically.Product invariants affected:
INV-MEM-1.Tests
backend/tests/unit/utils/test_listen_pusher_session.pycovers dead-socket invalidation, route-preserving replay, stable delivery IDs, cancellation, acknowledgement drain, and queue retention. Key cases includetest_finalization_send_failure_disconnects_dead_socket_and_preserves_request,test_ack_capability_retains_and_replays_transcript_until_peer_completion,test_full_speaker_buffer_preserves_accepted_unacknowledged_delivery, andtest_stale_socket_failure_does_not_disconnect_replacement.backend/tests/unit/test_listen_finalization_cloud_tasks.pyproves acknowledgement occurs only after owned effects complete, retryable speaker work remains unacknowledged, done markers suppress cross-socket duplicates, and a full stable queue does not evict accepted work.backend/tests/unit/test_redis_db_cache_serialization.pycovers lease ownership, done markers, owner-fenced release, bounded Redis timeouts, and fail-open behavior.backend/tests/unit/test_speaker_identification_delivery.pycovers retryable extraction outcomes, missing-person retry, passing a one-sample cap to the transactional append helper, and stable hashed object identity across retries.backend/tests/unit/test_async_webhooks.py,test_async_app_integrations.py,test_async_http_infrastructure.py,test_webhook_auto_disable.py, andtest_users_webhook_url_validation.pycover retry classification, stable idempotency keys, URL replacement, and explicit delivery-health reset without synthetic success.backend/tests/unit/test_memory_service_parity.py,test_memories_batch_delete.py,test_memories_delete_batch_chunk.py,test_memory_ledger.py, andtest_memories_batch.pycover progressive backfill, candidate caps, injected Firestore use, exact committed delete IDs, 499-write repair chunks, and reported vector write counts.backend/tests/unit/test_x_memory_extraction_retry.py::test_pending_x_source_remains_unacknowledged_after_partial_legacy_projectionproves partial legacy projection leaves the source retryable.backend/tests/unit/test_task_integrations_ops.pycovers missing identities, unexpected 2xx responses, rate limits, server failures, safe pre-send failures, and ambiguous post-send failures.test_task_integration_due_date_validation.py::test_provider_failure_metadata_is_preserved_in_public_responsecovers response propagation.backend/testing/e2e/test_task_integrations.pyverifies the expanded provider-create response at the HTTP boundary for success, provider500, and connect timeout outcomes.backend/testing/e2e/test_webhooks.pyexplicitly disables delivery after URL replacement and verifies a disabled webhook neither calls the provider nor mutates its reset health snapshot.backend/tests/unit/test_openapi_contract.py,test_app_client_ts_generator.py, andtest_app_client_dart_generator.pyverify the public schema and generated clients, includingtest_wrapped_task_integrations_wire_dart_is_generated_from_app_client_openapi.backend/tests/unit/test_dev_api_lock_bypass.pymodels the committed delete count and verifies the route injects its Firestore client.test_developer_memory_adapter.pyscopes the guard/read/delete ordering check toMemoryService.delete_external_memory.test_pusher_ghost_connections.pyparses the actual speaker-age condition instead of relying on line wrapping.test_tools_router.pyexercises the shared legacy-memory search boundary instead of stale vector internals.Verification
Commit
4ea54d07e5d3e3e03fb3a393fe06e0c1abc1b143has parenteb60a5175e96682185bba21829ccfeb98ba38c04, the latestorigin/mainused for publication. The final upstream advance touched no PR paths. The resulting patch is clean, and the complete listen session unit file passed again with27 passed.The maintainer-requested socket-identity regression is parameterized across receive and heartbeat failure paths:
For byte-identical prove-fail, only
backend/utils/listen_pusher_session.pywas restored fromorigin/main, andgit diff origin/main -- backend/utils/listen_pusher_session.pyprinted nothing before pytest ran. Both variants failed at the intended boundary with:Restoring the committed product file passed both variants. That product file is byte-identical between the prove-fail base and the final publication base.
Current-base behavioral verification:
Static and repository gates:
--line-length 120 --skip-string-normalization: all changed Python files accepted.INV-MEM-1, and required no failure-class declaration because nofix:commit is present.git diff --check: clean.The repository pre-push single-flight wrapper could not spawn its child process on Windows and exited with
WinError 2. Its underlying checks were run directly with the pinned project environments before the guarded force-with-lease publication.Failure-class protocol: no
fix:commit was used, so no declaration is required.Impact
User-visible successful outcomes stay the same; negotiated delivery paths now carry acknowledgement and stable identity metadata. The affected failure cases occur during ordinary reconnects, transient downstream failures, stale vector hydration, partial legacy X projection, or malformed provider success responses. They are not expected on every request, but their previous outcomes included dropped or duplicated side effects, leaked speaker objects, false task success, incomplete memory search, stale deleted projections, and legacy X sources acknowledged before all projections converged.
Limitations remain explicit: